-
-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Healpix Representation #178
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: nstarman <[email protected]>
I love this idea! I tried to do something like this many years ago, but never got it over the finish line. I am excited to review this. |
@lpsinger , do you want this in your 1.0 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This needs documentation.
- Can we support
nside
being an array? This would be important for supporting multi-resolution data sets. - Expose pixel area, resolution, etc. as instance methods.
- Indices should be of dtype
np.intp
so that they are suitable for indexing. What do we need to do to enforce that? Currently they are a (presumably dimensionless)u.Quantity
. - What should the units for
dx
anddy
be?
Hmm, I forgot about this! Do you think that this would eventually replace the existing high-level HEALPix class? |
_dx: u.Quantity | ||
_dy: u.Quantity | ||
|
||
@classmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I regret making this a classmethod. This should be a standalone function.
Thanks @lpsinger. I opened this PR mostly as a demonstration; I think it's a good idea, but requires a fair bit of work and time I don't have ATM. I'm happy to chat / review PRs.
Agreed.
Yes, with caveats. The
Sounds good. Though see the last point.
Aren't they already set as such? In self._indices = (self._indices << u.one).astype(int) So indices is a Quantity with dtype
Currently the Representation machinery requires all coordinates to be
Possibly! Or, like |
OK, in that case, I'll proceed with releasing v1.0.0 with what we have now. |
I don't have time to work on this much in the foreseeable future. So feel free to take this over the finish line or leave as is, no rush either way.
This PR is an early and incomplete implementation of healpix as a Representation. No more need for special methods like
xyz_to_healpix
. Using the representation machinery a healpix rep can transform to any Astropy representation, e.g. PhysicsSpherical. Also, since Representations can be inside a Frame or SkyCoord, this is super easy to work with.If implemented, this would be nice to use as the backend in
HEALPix
. IMO there's a lot of functions in this package that can be organized into methods on data structures. I've used some of them here, but the actual implementation could be moved to the Representation object and the separate function deprecated.Example usage:
Signed-off-by: nstarman [email protected]